refactor(event cache): a few renamings around the RoomEventCacheState and locks#6109
refactor(event cache): a few renamings around the RoomEventCacheState and locks#6109
RoomEventCacheState and locks#6109Conversation
…EventCacheState` There was nothing called `RoomEventCacheState` anymore, and the `Inner` suffix is dubious, at best. Also, we can get rid of the `Lock` component, since indeed it's locked, but it's a detail from the point of view of the `RoomEventCacheState` itself. This makes for a shorter and nicer name.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6109 +/- ##
=======================================
Coverage 89.84% 89.84%
=======================================
Files 363 363
Lines 100638 100638
Branches 100638 100638
=======================================
Hits 90420 90420
+ Misses 6690 6689 -1
- Partials 3528 3529 +1 ☔ View full report in Codecov by Sentry. |
Hywan
left a comment
Hywan
left a comment
There was a problem hiding this comment.
I like the renaming of RoomEventCacheStateLockInner to RoomEventCacheState.
I understand the motivation behind the simplification of the inner comments of the read operation, but I do not agree to remove the explanations. We've plenty of long inline comments in the code, for valid reasons. I consider this is a valid reason. I believe your additions is good, but the removals aren't.
Finally, I disagree with the renaming of the read lock acquisition.
| // Other attempts have been done in the past, including but not limited to | ||
| // using: an upgradeable read lock, a downgrading write lock, atomic | ||
| // read and write locks, and a semaphore with one permit. Each of | ||
| // these attempts had their own problems, so they've been ditched in | ||
| // favor of this solution. |
There was a problem hiding this comment.
I'm okay with this summary, but the details are important. Please restore the previous explanations.
There was a problem hiding this comment.
Alright, reverted this commit.
bnjbvr
force-pushed
the
bnjbvr/rename-complicated-struct-name
branch
from
bd64444 to
2034ea3
Compare
2 participants
I've also shortened the comment in the
readmethod, as talking about the previous implementations that didn't work in such detail doesn't help that much IMO. See the description commit by commit.